Skip to content

fix: rollback on drop so write conn returns to pool clean (#46)#47

Merged
jjhafer merged 1 commit intosilvermine:masterfrom
pmorris-dev:fix/interruptible-transaction-drop-leaks-writer
Apr 23, 2026
Merged

fix: rollback on drop so write conn returns to pool clean (#46)#47
jjhafer merged 1 commit intosilvermine:masterfrom
pmorris-dev:fix/interruptible-transaction-drop-leaks-writer

Conversation

@pmorris-dev
Copy link
Copy Markdown
Contributor

Dropping an ActiveInterruptibleTransaction without commit/rollback left the write connection in the pool with an open transaction. sqlx pools reuse connections rather than closing them, so SQLite's close-time auto-rollback never fired, and the next acquire_writer() got a connection where BEGIN IMMEDIATE failed with "cannot start a transaction within a transaction".

Drop now takes the writer and spawns a tokio task that issues ROLLBACK (and detach_if_attached) before the connection is released to the pool. The insert()/remove()/abort_all() paths that previously relied on Drop now roll back explicitly.

As defense-in-depth against any other writer path that might leak a transaction (e.g. user code that does BEGIN and returns early), the write pool gets an after_release hook that unconditionally runs ROLLBACK, ignoring the benign "no transaction is active" error.

Adds a regression test that mirrors the reporter's MCVE: start an interruptible transaction, drop it, start a second one, and assert the second succeeds and only its data commits.

Updates the README to document the auto-rollback-on-drop guarantee (including early returns via ?).

Comment thread crates/sqlx-sqlite-toolkit/src/transactions.rs Outdated
Comment thread crates/sqlx-sqlite-toolkit/src/transactions.rs Outdated
Comment thread crates/sqlx-sqlite-conn-mgr/src/database.rs Outdated
Comment thread crates/sqlx-sqlite-toolkit/src/transactions.rs Outdated
Comment thread crates/sqlx-sqlite-toolkit/src/transactions.rs
Comment thread crates/sqlx-sqlite-toolkit/src/transactions.rs Outdated
Comment thread crates/sqlx-sqlite-toolkit/src/transactions.rs Outdated
Comment thread crates/sqlx-sqlite-toolkit/tests/interruptible_transaction_tests.rs Outdated
)

Dropping an ActiveInterruptibleTransaction without commit/rollback left
the write connection in the pool with an open transaction. sqlx pools
reuse connections rather than closing them, so SQLite's close-time
auto-rollback never fired, and the next acquire_writer() got a
connection where BEGIN IMMEDIATE failed with "cannot start a
transaction within a transaction".

Drop now takes the writer and spawns a tokio task that issues ROLLBACK
(and detach_if_attached) before the connection is released to the pool.
The rollback task is bounded by a 5 s timeout so a stuck ROLLBACK
cannot hold the single-writer permit indefinitely. To eliminate a panic
risk at Drop time on threads without a tokio thread-local (notably
Tauri teardown after a programmatic app_handle.exit(N) on the main
thread), ActiveInterruptibleTransaction captures a tokio runtime handle
at construction and uses it unconditionally in Drop. The
insert()/remove()/abort_all() paths that previously relied on Drop now
roll back explicitly.

As defense-in-depth against any other writer path that might leak a
transaction (e.g. user code that does BEGIN and returns early), the
write pool gets an after_release hook that runs ROLLBACK. The hook
treats "no transaction is active" as the expected benign case and
instructs the pool to discard the connection on any other ROLLBACK
failure, so a broken connection is not handed to the next caller.

The plugin's ExitRequested handler no longer short-circuits on
programmatic exits (Some(code)). Treating those the same as
user-initiated exits means a user-space app_handle.exit(N) — fatal-
error handler, auto-updater, Ctrl+C handler — triggers transaction and
database cleanup instead of tearing down plugin state with live
interruptible transactions still in the map. The original exit code is
preserved through the cleanup detour via ExitGuard. CLEANUP_STATE == 2
is used to recognize the ExitGuard-driven re-exit and let it through.

Adds a regression test that mirrors the reporter's MCVE plus a second
test covering the attached-database drop path. Both wrap the second
begin_interruptible_transaction in tokio::time::timeout so a regression
in the drop path fails CI fast instead of hanging.

Updates the README to document the auto-rollback-on-drop guarantee
(including early returns via ?).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pmorris-dev pmorris-dev force-pushed the fix/interruptible-transaction-drop-leaks-writer branch from 0d9b8b8 to 2751290 Compare April 22, 2026 22:34
@pmorris-dev pmorris-dev requested review from jjhafer and yokuze April 22, 2026 22:37
@jjhafer jjhafer merged commit 00dfe10 into silvermine:master Apr 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants